-
Notifications
You must be signed in to change notification settings - Fork 2
Eye hand task #236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Eye hand task #236
Conversation
| This is for eye-related task that requires calibrated eye position | ||
| ''' | ||
|
|
||
| def __init__(self, task_params, data_queue, calibration_dir='/var/tmp', buffer_time=1, ylim=1, px_per_cm=51.67, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the unnecessary variables calibration_dir='/var/tmp', buffer_time=1, ylim=1, px_per_cm=51.67, since they're not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you can completely remove the init function here since it doesn't do anything anyway
| # Update eye diameter | ||
| if self.calibrated_eye_pos.size > 2: | ||
| self.temp = np.array(values[0])[[0,1,4]] | ||
| self.eye_diam = np.roll(self.eye_diam, -1, axis=0) | ||
| self.eye_diam[-1] = self.temp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calibrated_eye_pos is never more than 2 elements, but we still get eye pos which contains diameter
| # Update eye diameter | |
| if self.calibrated_eye_pos.size > 2: | |
| self.temp = np.array(values[0])[[0,1,4]] | |
| self.eye_diam = np.roll(self.eye_diam, -1, axis=0) | |
| self.eye_diam[-1] = self.temp | |
| elif key == 'eye_pos': | |
| # Update eye diameter | |
| if self.eye_pos.size > 2: | |
| self.temp = np.array(values[0])[[0,1,4]] | |
| self.eye_diam = np.roll(self.eye_diam, -1, axis=0) | |
| self.eye_diam[-1] = self.temp |
| self.diam_plot.set_data(np.arange(len(self.eye_diam)) * 1/(int(self.task_params['fps'])) - self.buffer_time, | ||
| self.eye_diam[:, 2]/self.px_per_cm) | ||
|
|
||
| class EyeHandAnalysisWorker(SaccadeAnalysisWorker): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to inherit SaccadeAnalysisWorker or can you inherit from BehaviorAnalysisWorker instead to simplify the inheritance?
| @staticmethod | ||
| def out_2D_square(nblocks=100, width=10, height=10, origin=(0,0,0)): | ||
| ''' | ||
| Generates a sequence of 2D (x and z) targets at a point on the side of the square |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you be a bit more descriptive? How many positions will there be? What exactly does it mean to be a point on the side of the square?
| #self.cube.color = RED | ||
| #self.show() | ||
| pass | ||
|
|
||
| def cue_trial_end_success(self): | ||
| self.cube.color = GREEN | ||
|
|
||
| def cue_trial_end_failure(self): | ||
| self.cube.color = YELLOW | ||
| self.hide() | ||
| self.cube.color = RED | ||
| #self.hide() | ||
|
|
||
| def idle(self): | ||
| self.cube.color = RED | ||
| #self.cube.color = RED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't understand these changes, can you explain why you needed to do this? won't it affect other tasks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It affects other tasks, but I changed this so that the cube target can change its color in the same way as the circular target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah got it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the comments
leoscholl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i reviewed the changes to online_analysis.py
i had a hard time following the eye/hand tasks because they are so mixed up with the target capture task. i think it would be better if they were completely separate tasks. what do you think?
I agree that separate tasks should be better for visuality and interpretability. I was just lazy because some states like wait and penalty are the same. |
|
I started to make this eye-hand task separate from the target capture task. But I noticed that many states overlapped between the 2 tasks. For example, the penalty states are almost similar. And also sometimes copying code itself is hard because I need to see both the screen target capture task and the target capture task. So how about making new code without inheritance just for important states such as wait, target, delay, and so on, so that it can get easier for people to understand state flows. I want to inherit other penalty states. I think this is a compromised solution. What do you think? |
|
ok well don't worry about it then. |
|
|
||
| yield indices, targs | ||
|
|
||
| class EyeConstrainedHandCapture(HandConstrainedEyeCapture): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a description of the changes
|
|
||
| class EyeHandSequenceTask(ManualControlMixin, EyeHandSequenceCapture): | ||
| ''' | ||
| Saccade task while holding different targets by hand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't the correct description i don't think.
| # Target 1 and 2 are for saccade. Target 3 is for hand | ||
| target1 = VirtualCircularTarget(target_radius=self.fixation_radius, target_color=target_colors[self.eye_target_color]) | ||
| target2 = VirtualCircularTarget(target_radius=self.fixation_radius, target_color=target_colors[self.eye_target_color]) | ||
| target1 = VirtualRectangularTarget(target_width=self.fixation_radius, target_height=self.fixation_radius/2, target_color=target_colors[self.eye_target_color]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is height = width/2
| target_pos = np.delete(self.targs[self.target_index],1) | ||
| d_eye = np.linalg.norm(eye_pos - target_pos) | ||
| return (d_eye <= self.fixation_radius + self.fixation_radius_buffer) or self.pause | ||
| eye_d = np.linalg.norm(eye_pos - self.targs[self.target_index,[0,2]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you can be bothered, it would be better if self.calibrated_eye_pos was 3D rather than ignoring the Y component of the target. For VR experiments this might be important
| Then they need to fixate the first eye target and make a saccade for the second eye target. 2 of chain_length is only tested. | ||
| ''' | ||
|
|
||
| fixation_radius = traits.Float(2.5, desc="Distance from center that is considered a broken fixation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update description: it's now the radius (width?) of the fixation target
| eye_target_color = traits.OptionsList("white", *target_colors, desc="Color of the eye target", bmi3d_input_options=list(target_colors.keys())) | ||
| fixation_target_color = traits.OptionsList("fixation_color", *target_colors, desc="Color of the eye target under fixation state", bmi3d_input_options=list(target_colors.keys())) | ||
| eye_target_color = traits.OptionsList("eye_color", *target_colors, desc="Color of the eye target", bmi3d_input_options=list(target_colors.keys())) | ||
| fixation_radius_buffer = traits.Float(.5, desc="additional radius for eye target") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update description: the sum of the fixation radius and the buffer is what determines the break of fixation
| fixation_target_color = traits.OptionsList("fixation_color", *target_colors, desc="Color of the eye target under fixation state", bmi3d_input_options=list(target_colors.keys())) | ||
| eye_target_color = traits.OptionsList("eye_color", *target_colors, desc="Color of the eye target", bmi3d_input_options=list(target_colors.keys())) | ||
| fixation_radius_buffer = traits.Float(.5, desc="additional radius for eye target") | ||
| fixation_time = traits.Float(.2, desc="additional radius for eye target") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update description
| eye_target_color = traits.OptionsList("eye_color", *target_colors, desc="Color of the eye target", bmi3d_input_options=list(target_colors.keys())) | ||
| fixation_radius_buffer = traits.Float(.5, desc="additional radius for eye target") | ||
| fixation_time = traits.Float(.2, desc="additional radius for eye target") | ||
| incorrect_target_radius_buffer = traits.Float(.5, desc="additional radius for eye target") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update description
| fixation_time = traits.Float(.2, desc="additional radius for eye target") | ||
| incorrect_target_radius_buffer = traits.Float(.5, desc="additional radius for eye target") | ||
| incorrect_target_penalty_time = traits.Float(1, desc="Length of penalty time for acquiring an incorrect target") | ||
| exclude_parent_traits = ['hold_time'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to add a hidden_traits list for the above that are not commonly used - at least eye_target_color and fixation_target_color i think
|
|
||
| # Initialize fixation state | ||
| self.num_hold_state = 0 | ||
| self.isfixation_state = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this variable could be better named, it's not immediately clear what this means. maybe avoid using state since that's confusing with the state machine. i suggest fixation_acquired or is_fixated
| target = self.targets[next_idx % 2] | ||
| target.move_to_position(self.targs[next_idx % 2]) | ||
| target = self.targets[next_idx] | ||
| target.move_to_position(self.targs[next_idx] - self.offset_cube) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the purpose of the offset?
| def _end_reward(self): | ||
| super()._end_reward() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can delete
| instantiate_targets = kwargs.pop('instantiate_targets', True) | ||
| if instantiate_targets: | ||
|
|
||
| # Target 1 and 2 are for saccade. Target 3 is for hand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about target 4?
| ''' | ||
|
|
||
| exclude_parent_traits = ['delay_time', 'rand_delay'] | ||
| rand_delay1 = traits.Tuple((0.4, 0.7), desc="Delay interval for eye") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you call these rand_delay_eye and rand_delay_hand instead of 1 and 2
| ) | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parent class already makes targets, i think you should use
super(ScreenTargetCapture, self).__init__(*args, **kwargs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same applies to all your other tasks
| class ScreenTargetCapture_Saccade(ScreenTargetCapture): | ||
| ''' | ||
| Center-out saccade task. The controller for the cursor position is eye position. | ||
| Hand cursor is also visible. You should remove the hand cursor by setting cursor_radius to 0 as needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Hand cursor is also visible. You should remove the hand cursor by setting cursor_radius to 0 as needed. | |
| Hand cursor is also visible. You should remove the hand cursor by plant_visible to False as needed. |
| self.show() | ||
| #self.cube.color = RED | ||
| #self.show() | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think we use this function but it should be
self.cube.color = self.target_color
self.show()
Sorry for a big PR.
Main changes are below,